Implement fuzz testing #97

ionut-arm · 2020-01-31T11:34:04Z

This commit implements fuzz testing through libFuzzer and the framework around it
to continuously run the fuzzer in a Docker container, along with a service that
checks for updates on the repo.
The service builds the Docker image and launches the fuzzer, after which it waits
for any update to the service. Given that the fuzzing corpus will remain in-place,
whenever fuzzing is restarted for another service version, it will take off from the
already existing cases.
The Docker image has all the required components to run all the current providers -
Mbed, PKCS11 and TPM - and to continuously fuzz the service through a stub version of
the frontend handler. If a crash is detected, the fuzzer will notify the team and
continue fuzzing.

hug-dev

Thanks a lot for this PR, this is very good and so helpful.
Added a few questions/comments. My biggest change request would be to streamline this to remove the parsec-interface/fuzz feature (as my supposition is that it's no longer needed) and remove associated code change.
It could still be added later when other fuzz targets are added!

hug-dev · 2020-02-03T11:02:26Z

fuzz/run_fuzz.sh

+ cargo +nightly fuzz run fuzz_service
+
+ # Notify about crash
+ echo "Here we'd ping the webhook to notify"


As we currently do not notify the crash, would it be easy to read the logs to find the crashes that happened? Could it be easier, for now, to remove the while true loop and exit at the first crash?

would be, but why not continue? the crash data (i.e. the input that triggered the crash) is saved by the fuzzer anyway. So far the only crash that happened was because the fuzzer kept using up more and more memory and died when it reached its limit - not exactly a meaningful crash

That's good then! As long as it is easy to know when a crash happenened and with what data.

It doesn't only record crashes, it also records slow runs

hug-dev · 2020-02-03T11:06:19Z

Cargo.toml

@@ -49,3 +50,4 @@ default = ["mbed-crypto-provider", "pkcs11-provider"]
 mbed-crypto-provider = []
 pkcs11-provider = ["pkcs11", "picky-asn1-der", "picky-asn1"]
 tpm-provider = ["tss-esapi", "picky-asn1-der", "picky-asn1"]
+fuzz = ["parsec-interface/fuzz", "arbitrary"]


Is the parsec-interface/fuzz feature still needed now? As we only pass a stream to the fuzzer and now longer a Request + ApplicationName?
I guess as discussed in #54 it will be used when fuzzing individual components, but for now should we remove it?

yeah, i guess I can remove it and reintroduce when adding the other fuzzing stuff

hug-dev · 2020-02-03T11:08:22Z

fuzz/Cargo.toml

+[package]
+name = "parsec-fuzz"
+version = "0.0.0"
+authors = ["Automatically generated"]
+publish = false
+edition = "2018"


Do we care about filling those with the same values as in our other crates?

wasn't sure about that... it was obviously automatically generated, I can update it to be something similar to the parsec Cargo.toml

I think it would be good

I'll only update the authors, will leave version as "0.0.0"

hug-dev · 2020-02-03T11:10:26Z

fuzz.sh

+ docker logs -f --tail 100 $FUZZ_CONTAINER_NAME
+elif [[ "$1" == "clean" ]]
+then
+ docker run -d --rm -v $(pwd):/parsec -w /parsec/fuzz --name $CLEANUP_CONTAINER_NAME parsec/fuzz ./cleanup.sh


The cleanup.sh script seems to only have a dependency on the rm command. Do we need to execute it inside the parsec/fuzz container? We could also write the commands directly here and remove the script.

well yes, but no. if you try that on your server, in the parsec account, you'll see why :) when docker runs, it runs as root, so removing new files isn't possible from a non-sudoer account, unless you do something from a container again

I'll add a comment!

Thanks for the info!

hug-dev · 2020-02-03T11:13:08Z

fuzz/fuzz_targets/fuzz_service.rs

+use parsec::authenticators::ApplicationName;
+use parsec::fuzz_utils::requests::Request;


I don't think these two use are needed anymore. cf the comment on the parsec-interface-rs fuzz feature :)

hug-dev · 2020-02-03T11:20:55Z

fuzz/fuzz_targets/fuzz_service.rs

+#[derive(Arbitrary, Debug)]
+struct MockStream(Vec<u8>);
+
+impl Read for MockStream {
+ fn read(&mut self, buf: &mut [u8]) -> Result<usize> {
+ if self.0.is_empty() {
+ return Ok(0);
+ }
+ let n = cmp::min(buf.len(), self.0.len());
+ for (idx, val) in self.0.drain(0..n).enumerate() {
+ buf[idx] = val;
+ }
+
+ Ok(n)
+ }
+}
+
+impl Write for MockStream {
+ fn write(&mut self, buf: &[u8]) -> Result<usize> {
+ Ok(buf.len())
+ }
+
+ fn flush(&mut self) -> Result<()> {
+ Ok(())
+ }
+}
+
+fuzz_target!(|stream: MockStream| {
+ FRONT_END_HANDLER.handle_request(stream);
+});


That is really awesome 💯
Because of the MockStream Read + Write interface and the modularity of the design this is amazing to see how easy it is to re-use almost everything in Parsec!

hug-dev · 2020-02-03T11:23:32Z

fuzz/run_fuzz.sh

+set -e
+set -x


As we were discussing it and just FYI, I was reading this interesting article about safe shell scripting and the recommended variables to set on top of shell scripts. Not requesting any changes here.

hug-dev · 2020-02-03T11:25:00Z

src/authenticators/mod.rs

+#[cfg(feature = "fuzz")]
+use arbitrary::Arbitrary;
 use parsec_interface::requests::request::RequestAuth;
 use parsec_interface::requests::Result;

+#[cfg_attr(feature = "fuzz", derive(Arbitrary))]


Are those still needed?

hug-dev · 2020-02-03T11:25:13Z

src/lib.rs

+
+#[cfg(feature = "fuzz")]
+pub mod fuzz_utils {
+ pub use parsec_interface::*;
+}


Same here: is it still needed?

hug-dev · 2020-02-03T11:26:57Z

fuzz.sh

+if [[ "$1" == "run" ]]
+then
+ # Set up fuzz folder
+ cp fuzz/config.toml fuzz/run_config.toml


Why is it needed to copy the configuration file?

so that you don't have to remove the PKCS11 slot number after each use

Ah makes sense! Maybe a comment would help here then

This commit implements fuzz testing through libFuzzer and the framework around it to continuously run the fuzzer in a Docker container, along with a service that checks for updates on the repo. The service builds the Docker image and launches the fuzzer, after which it waits for any update to the service. Given that the fuzzing corpus will remain in-place, whenever fuzzing is restarted for another service version, it will take off from the already existing cases. The Docker image has all the required components to run all the current providers - Mbed, PKCS11 and TPM - and to continuously fuzz the service through a stub version of the frontend handler. If a crash is detected, the fuzzer will notify the team and continue fuzzing. Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>

hug-dev

Thanks for the changes! Looking good

ionut-arm added the enhancement New feature or request label Jan 31, 2020

ionut-arm added this to the Parsec production ready milestone Jan 31, 2020

ionut-arm requested review from hug-dev and paulhowardarm January 31, 2020 11:34

ionut-arm self-assigned this Jan 31, 2020

hug-dev reviewed Feb 3, 2020

View reviewed changes

ionut-arm force-pushed the fuzz branch from e1b7d4e to 1339df8 Compare February 3, 2020 13:05

hug-dev approved these changes Feb 3, 2020

View reviewed changes

ionut-arm merged commit f9b496b into parallaxsecond:master Feb 3, 2020

ionut-arm deleted the fuzz branch February 3, 2020 13:56

ionut-arm mentioned this pull request Feb 3, 2020

Add fuzz tests #54

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement fuzz testing #97

Implement fuzz testing #97

ionut-arm commented Jan 31, 2020

hug-dev left a comment

hug-dev Feb 3, 2020

ionut-arm Feb 3, 2020

hug-dev Feb 3, 2020

ionut-arm Feb 3, 2020

hug-dev Feb 3, 2020

ionut-arm Feb 3, 2020

hug-dev Feb 3, 2020

ionut-arm Feb 3, 2020

hug-dev Feb 3, 2020

ionut-arm Feb 3, 2020

hug-dev Feb 3, 2020

ionut-arm Feb 3, 2020

ionut-arm Feb 3, 2020

hug-dev Feb 3, 2020

hug-dev Feb 3, 2020

hug-dev Feb 3, 2020

hug-dev Feb 3, 2020

hug-dev Feb 3, 2020

ionut-arm Feb 3, 2020

hug-dev Feb 3, 2020

hug-dev Feb 3, 2020

ionut-arm Feb 3, 2020

hug-dev Feb 3, 2020

hug-dev left a comment

		use parsec::authenticators::ApplicationName;
		use parsec::fuzz_utils::requests::Request;

Implement fuzz testing #97

Implement fuzz testing #97

Conversation

ionut-arm commented Jan 31, 2020

hug-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hug-dev left a comment

Choose a reason for hiding this comment